Repository navigation
The offload probe has to report when the link is what ends it - #1749
Merged
Merged
Conversation
First field run of the unbonded offload probe. It got its answer and exposed two defects in itself doing it. The capture: 16 probe starts, ZERO verdicts of any kind, zero confirmed subscribes, zero refusals, and every link dying 10.8s into the connect - about three seconds after the CCCD writes went out. Stage 1 ended with the LINK, an exit the probe had no branch for, so it concluded nothing, the silence budget never advanced, and it re-ran on every reconnect indefinitely. That is precisely the unbounded retry shouldProbeUnbondedOffload's own doc claims this design prevents, reintroduced through an unhandled exit - and it cost the user a strap bouncing every 17 seconds all night. That exit is now a verdict, charged to the silence budget, because "the link will not survive being asked" is a stronger reason to stop asking than a strap that merely stayed quiet. It is also a FINDING and the line says so. No callback and no ATT error, then a teardown about three seconds later, is the CLIENT_HELLO's own signature on the same service. It does not prove the puffin characteristics require encryption, but it is what that would look like from here, and it says plainly that the offload is not reachable on this strap without a bond. The probe was built to answer that question either way, and this is the answer. Second defect, in the line the capture is read through: the supersede message warned that "Ask Android to pair" being on makes a refusal unattributable - while the very same branch returns BEFORE the pairing request, so no SMP can be in flight. The capture confirms it: not one probe connect carries a createBond. A caveat that could not apply, printed on every probe connect, and it briefly cast doubt on a clean result while reading this log. It now says the pairing is also skipped and the refusal IS attributable. The test that pinned the old wording asserted "unattributable" and had to be repinned - the same shape as the writeClientHello contract test earlier in this thread. A test can be correct and then quietly become a guard on the wrong behaviour. Staging 394 / iOS 275. 4912 tests, lintVital, doc lint and i18n clean.
Re-review of the link-lost verdict. The commit above closed stage 1's silent exit and left stage 2's open, immediately underneath it. The verdict timer is cancelled on teardown, so a link lost during the GET_CLOCK wait reported nothing and spent no budget attempt either - the unbounded retry reopened one stage down from where it had just been closed. Both stages now report and both spend an attempt. They get DIFFERENT lines, and that is the substance rather than the tidiness. A stage-1 loss carries a finding: the subscribes drew no callback AND no ATT error before the drop, which is the CLIENT_HELLO's own signature on the same service. A stage-2 loss carries none: the subscribes landed, so the transport was open and the strap was still inside its window to answer. Reusing stage 1's wording there would manufacture evidence out of an inconclusive link, which is the exact conflation this probe has now had to unpick three times - the false ack, the write-completion bond, and this. Verified while re-reviewing: connectedAtMs is not cleared by reset(), so the uptime this reports is the dropped link's own, not a stale one; handleDisconnect's reset clears the flags before connectToDevice's can see them, so a loss is charged once; and once the budget retires the probe the supersede branch keeps the hello and the pairing request off, which leaves the strap in the stable live-HR state rather than back in the 4.8s hello loop. 4913 tests, lintVital, doc lint and i18n clean.
ryanbr
pushed a commit
that referenced
this pull request
Aug 31, 2026
…1746) GET_DATA_RANGE answers twice - a short PENDING(2) acknowledgement, then the payload with SUCCESS(1) - and the #689 backlog decode ran on both. The ack carries no ring pointers by construction, so it decoded to null and logged "offsets may have moved" once per sync on healthy hardware, pointing every reader at an alignment bug that was not there. The tree already knew: Framing's own result-code table records "2=PENDING precedes SUCCESS on GET_DATA_RANGE (hardware-confirmed, #78 fork)". The decode site simply had no way to act on it, because a too-short frame and a misaligned one look the same to pagesBehind. A pure isPendingResponse(frame, cmdOff) on both platforms reads the result byte at cmdOff+2 and gates the decode at the two call sites. The SUCCESS frame that will not parse still logs exactly as loudly as before - that is the case #818's comment protects, and it is untouched. The #451 raw dump stays unconditional above it. Verified against the captured bytes rather than the description. On the PENDING frame, index 10 is 0x22 (34, GET_DATA_RANGE), 11 is the origin sequence, 12 is 0x02 - the cmdOff+2 claim is exact. That fixture is a whole frame: declLen 12, +8, and it is 20 bytes. The SUCCESS fixture is trimmed from its declared 84 bytes to 40, which is deliberate rather than damaged: pagesBehind reads to index 37, so the trim keeps precisely what the decoder needs, and decoding it by hand gives w=48833, u=48820, t=131072 -> 13, matching the log excerpt in the PR. The two predicates are exact twins, including the detail that Kotlin masks the signed Byte where Swift's UInt8 does not need it. Five new cases per platform, one-to-one. Merged onto a main that had moved a long way under it - WhoopBleClient.kt was rewritten substantially by #1744, #1748 and #1749 after this branch was cut - so it was merged locally and run rather than trusted: 4,926 tests, 0 failures, DataRangeScanTest 17/17, doc lint and i18n clean, all fifteen CI checks green. One thing left on the table for later, since the fixture already supports it: nothing asserts pagesBehind == 13 on that SUCCESS frame. The pair currently pins only that the two are distinguishable; asserting the value would pin that the pair behaves exactly as the log shows, for free, on both platforms. The PR body was edited before merge to remove a closing verb adjacent to an issue number. It was a denial - the contributor was explicitly saying the issue should stay open - but GitHub matches those keywords literally and does not read negations, so it would have been actioned on merge. Harmless this time only because that issue was already closed. Thanks @Zebsi235 - real captured frames rather than synthetic ones, the failing stub run to prove the tests actually catch the regression, and the Swift gap stated up front rather than left to be found.
ryanbr
added a commit
that referenced
this pull request
Aug 31, 2026
) Found in the 13:17 capture, and it was my own scheduling rather than the strap. DIS reads and CCCD writes share ONE serialized GATT queue. The probe was posted on a fixed 6s delay, chosen on the reasoning that the DIS chain - posted at 3s - would be done by then. On one link it was still running at 7s: 07:57:06 writeDescriptor busy for fd4b0003 ... retry 1/8 07:57:06 ... 8 busy retries across the four chars, shared budget exhausted 07:57:06 writeDescriptor rejected for fd4b0003/4/5/7 (gave up after 8 retries) 07:57:06 Unbonded offload probe: none of the 4 ... completed their subscribe, and none was refused either ... proves nothing either way 07:57:07 DIS: modelNumber=MG <- the chain, still going Not one descriptor write reached the air. Reasoning about a delay lost to measuring one. The probe now waits on the actual signal, checked once a second and bounded at eight checks, then goes anyway and says which happened. The same log also handed over a control case that #1749 asserted without one. Every link whose CCCD writes reached the air died at 10.8s - 4.8s after the probe, by localTerminate, the CLIENT_HELLO's own signature. The ONE link where all four writes were abandoned locally lived 31.3s and died of an ordinary connectionTimeout. The teardown tracks the write reaching the strap, not the probe running. Three review passes, and all three found the same shape of defect: a flag claiming something that had not happened. 1. the contention itself 2. disChainInFlight was set BEFORE readDisIdentity, which has five early returns above the read it issues plus an unavailable-characteristic branch - so on any of those the probe stood aside for a chain that never started. readDisIdentity now reports whether it ISSUED a read, and the flag is set from that answer rather than from the intention to ask. 3. readNextDisExtra is only reached from the SUCCESS path, so a REFUSED read ended the chain while the flag stayed set - and #490's whole subject is a strap that refuses DIS, so that was the case the probe most wanted a clear queue for. All three were survivable because the cap bounds them, which is exactly why they would have gone unnoticed: a wrong wait that self-corrects looks like a working wait. The wait decision also moved into a pure helper beside the rest of this file's judgements. It was inline in the client - argued for in a comment and asserted nowhere - which is precisely how #1755 shipped a bound every unit test agreed with and the real pipeline rejected. Parity checked rather than assumed: the DisUnbondedRead pure twin pair is untouched, four functions each side, and Swift performs its DIS read inline with no readDisIdentity counterpart, so the Bool return has no twin to keep in step. That structural difference predates this work. Everything else is the probe, Android-only by design. Staging 396 / iOS 277. 4,941 tests, zero failures. Doc lint and i18n clean. The next capture is what confirms the probe now starts on a clear queue.
ryanbr
added a commit
that referenced
this pull request
Aug 31, 2026
…ned (#1635) The give-up line said the strap "serves those characteristics unbonded but does not act on commands written to them". That holds only for links that subscribed and then stayed quiet. Since #1749 a link LOST mid-probe charges the same budget, and those links confirm no subscribes at all — so on the 31 Aug capture, where every charge was a lost link, retiring the probe would have recorded the exact opposite of what that strap demonstrated. It is the wrong line to overclaim on. The retirement line is where a future reader goes to find out whether the offload is possible at all, and reading the stronger evidence off the weaker one is the mistake this file was built to avoid — it is what the false bond of 28 Aug was. The line now names both ways the budget can be spent and asserts neither, since the budget does not distinguish them. Nothing is lost: the per-link lines already record which happened each time, with the confirmed subscribe count. The test that pinned the old wording pinned a false claim. Repinned to the same shape: both possibilities named, neither asserted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First field run of the unbonded offload probe (#1742/#1743). It got its answer, and exposed two defects in itself doing it.
What the capture says
Every connect: handshake skipped → probe queues the four puffin CCCDs at +6s → link gone at +10.8s, roughly three seconds later. Nothing subscribed, nothing refused, nothing concluded.
Defect 1 — stage 1 can end with the LINK, and there was no branch for it
The probe concluded nothing, so the silence budget never advanced, so it re-ran on every reconnect indefinitely. That is exactly the unbounded retry
shouldProbeUnbondedOffload's own doc claims this design prevents, reintroduced through an unhandled exit — and it cost a strap bouncing every 17 seconds all night.That exit is now a verdict, charged to the silence budget: "the link will not survive being asked" is a stronger reason to stop asking than a strap that merely stayed quiet.
And it is a finding, not just a gap. No callback and no ATT error, then a teardown ~3s later, is the
CLIENT_HELLO's own signature on the same service. It does not prove the puffin characteristics require encryption, but it is what that would look like from here — and it says plainly that the offload is not reachable on this strap without a bond. The probe was built to answer that either way; this is the answer.Defect 2 — the line the capture is read through carried a caveat that could not apply
The supersede message warned that "Ask Android to pair" being on makes a refusal unattributable — while the same branch returns before the pairing request. The capture confirms it: not one probe connect carries a
createBond.So every probe connect printed a caveat that was structurally impossible, and it briefly cast doubt on a clean result while reading this log. It now says the pairing is also skipped and the refusal is attributable.
A note on the test
The test pinning the old wording asserted
"unattributable"and had to be repinned — the same shape as thewriteClientHellocontract test earlier in this thread. A test can be correct and then quietly become a guard on the wrong behaviour.Verification
4912tests, 0 failureslintVitalFullRelease, doc-comment lint, i18n--cicleanRelated to #1635.